Skip to content

std.os.linux: remove app_mask and block&unblock pair in raise #22337

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 2, 2025

Conversation

ruihe774
Copy link
Contributor

I believe the order of first two u32 is wrong.

See also: https://github.com/bminor/musl/blob/master/src/signal/sigfillset.c

@alexrp
Copy link
Member

alexrp commented Dec 28, 2024

The reason musl special-cases SIGTIMER (32), SIGCANCEL (33), and SIGSYNCCALL (34) is that they're used in the implementation of pthread and timer APIs in musl.

The libc-less version of Zig's standard library does not use these signal numbers for anything, and the app_mask and all_mask constants are only used in the Linux implementations of raise() and abort() in std.posix when libc is not linked.

I'm inclined to say that, rather than adjusting app_mask, it should just be deleted in favor of using all_mask in raise() instead. But maybe there's some factor I'm missing here. cc @andrewrk since these constants were added in 6f1a7a0.

@ruihe774
Copy link
Contributor Author

I'm wondering why raise() need to block and unblock signals. musl does this, but glibc does not. I think we can remove the block&unblock pair.

@ruihe774 ruihe774 changed the title std.os.linux: fix value of app_mask std.os.linux: remove app_mask and block&unblock pair in raise Dec 31, 2024
@alexrp
Copy link
Member

alexrp commented Dec 31, 2024

musl does this, but glibc does not. I think we can remove the block&unblock pair.

Maybe. I'd still like Andrew's thoughts on this before we go ahead with that change.

@alexrp
Copy link
Member

alexrp commented Dec 31, 2024

Also, perhaps the same would be true of abort()?

@ruihe774
Copy link
Contributor Author

ruihe774 commented Dec 31, 2024

Also, perhaps the same would be true of abort()?

Current implementation of abort() first uses raise() then goes through the block/unblock pair. I guess in most cases (no user-installed handler) the process will just terminate at the first raise(). If it is not, it is necessary to use the block/unblock pair and reset the signal handler when all signals are blocked to avoid race condition.

@ruihe774
Copy link
Contributor Author

ping @andrewrk.

@ruihe774
Copy link
Contributor Author

Any updates on this? @alexrp @andrewrk

@alexrp
Copy link
Member

alexrp commented Apr 2, 2025

I'm wondering why raise() need to block and unblock signals. musl does this, but glibc does not. I think we can remove the block&unblock pair.

https://git.musl-libc.org/cgit/musl/commit/?id=0bed7e0acfd34e3fb63ca0e4d99b7592571355a9

This actually seems like a pretty good reason to keep the signal blocking.

However, we can just block all signals rather than only blocking "application signals" as defined by musl. musl makes this distinction because musl knows that the signal handlers for its internal signals (SIGTIMER, SIGCANCEL, and SIGSYNCCALL) are well-behaved and won't fork(). So it's just just a minor optimization AFAICT, and again, we don't have a concept of internal signals in the libc-less Zig standard library.

@ruihe774
Copy link
Contributor Author

ruihe774 commented Apr 2, 2025

I'm wondering why raise() need to block and unblock signals. musl does this, but glibc does not. I think we can remove the block&unblock pair.

https://git.musl-libc.org/cgit/musl/commit/?id=0bed7e0acfd34e3fb63ca0e4d99b7592571355a9

This actually seems like a pretty good reason to keep the signal blocking.

However, we can just block all signals rather than only blocking "application signals" as defined by musl. musl makes this distinction because musl knows that the signal handlers for its internal signals (SIGTIMER, SIGCANCEL, and SIGSYNCCALL) are well-behaved and won't fork(). So it's just just a minor optimization AFAICT, and again, we don't have a concept of internal signals in the libc-less Zig standard library.

Modified.

@alexrp alexrp enabled auto-merge April 2, 2025 10:54
@alexrp alexrp added this to the 0.14.1 milestone Apr 2, 2025
@alexrp alexrp merged commit 9dfdf35 into ziglang:master Apr 2, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants